pkg/asset/installconfig/ssh: Add OPENSHIFT_INSTALL_SSH_PUB_KEY_PATH #351
Conversation
openshift-ci-robot
added
approved
pkg/asset/installconfig/ssh.go
Outdated
There was a problem hiding this comment.
We still need to allow the user to opt out of using a key at all.
There was a problem hiding this comment.
We still need to allow the user to opt out of using a key at all.
I think we do. Some cases:
- You set
OPENSHIFT_INSTALL_SSH_PUB_KEYand exit up here. - You set
OPENSHIFT_INSTALL_SSH_PUB_KEY_PATH, get that single entry in yourpubKeys, and exit here without a prompt. - You set no variables, so you get the
<none>choice injected. Then...- Go finds no valid pubkeys in
~/.ssh/*.pub, and you exit here with the none choice selected. - Go finds some valid pubkeys in
~/.ssh/*.pub, and you pass by here and fall down to the interactive prompt.
- Go finds no valid pubkeys in
There was a problem hiding this comment.
Ah, that seems okay then.
By switching to survey.Question which has room for a validator [1]. I've also pushed the Required validator up into the per-property config, so we don't bake requiredness into UserProvided itself. [1]: https://godoc.org/github.com/AlecAivazis/survey#Question
Tables are annoying to maintain in ASCII, and lists give us more space to talk about the expected input format, etc.
To make it easier to pass this in when you don't have a shell for: $ OPENSHIFT_INSTALL_SSH_PUB_KEY="$(cat path/to/key)" openshift-install install-config For example, this is useful in CI where we can launch the cluster without hitting a shell at all. And it's also a bit more compact for folks who are reading from files anyway. I've also added a check for "do we only have one choice?". If so, I just pick that value instead of bothering the user when they don't have a decision to make.
wking
force-pushed
the
ssh-path-from-env-var
branch
from
005a175 to
c2f93fd
Compare
|
@wking If you rebase this, I'll LGTM it. |
Does it need a rebase? |
|
I just want to be sure that this has the changes that I requested back in #350. This should just be two commits. |
|
Nevermind, I see the commit hash is the same. /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: crawford, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
On second thought, can we just drop |
We could, I just don't like touching the disk if the caller already happens to have this in memory. But having this in memory is unlikely (maybe for unit tests?), so I'm ok dropping it if you want.
With some variables read directly and some read from paths, at least #352 is going to need to support both approaches. The SSH reader could be simplified slightly if we only supported one approach, but I think it's going to be a difference of ~10 lines. Is that too much? |
|
/retest #352 was happy (before I had to push to fix |
I'm not concerned with the amount of code so much as I'm concerned about supporting all of these interfaces. |
|
In the interest of time, let's leave this as is. We can clean it up after CI is working. |
|
@wking: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
5 participants
Builds on #350; review that first.
To make it easier to pass this in when you don't have a shell for:
$ OPENSHIFT_INSTALL_SSH_PUB_KEY="$(cat path/to/key)" openshift-install install-configFor example, this is useful in CI where we can launch the cluster without hitting a shell at all. And it's also a bit more compact for folks who are reading from files anyway.
I've also added a check for "do we only have one choice?". If so, I just pick that value instead of bothering the user when they don't have a decision to make.
And I've reformatted the docs to use a list. Tables are annoying to maintain in ASCII, and lists give us more space to talk about the expected input format, etc.
/assign @crawford